[mlir][tosa] Add missing verifier for tosa.pad
#120934
Conversation
|
@llvm/pr-subscribers-mlir-tosa Author: Longsheng Mou (CoTinker) ChangesThis PR adds a missing verifier for Full diff: https://github.com/llvm/llvm-project/pull/120934.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index 631d3c48f2df02..6d9d2badc2970b 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -831,6 +831,11 @@ LogicalResult tosa::PadOp::verify() {
if (paddingType.hasRank() && paddingType.getRank() != 2)
return emitOpError() << "expect 'padding' tensor rank equal to 2.";
+ auto paddingShape = paddingType.getShape();
+ if (paddingShape.front() != inputType.getRank() || paddingShape.back() != 2)
+ return emitOpError()
+ << "expect 'padding' tensor shape to match [rank(input), 2]";
+
return success();
}
diff --git a/mlir/test/Dialect/Tosa/invalid.mlir b/mlir/test/Dialect/Tosa/invalid.mlir
index cca50b25d14d6b..8a8fcbed6a67f3 100644
--- a/mlir/test/Dialect/Tosa/invalid.mlir
+++ b/mlir/test/Dialect/Tosa/invalid.mlir
@@ -119,6 +119,22 @@ func.func @test_pad_invalid_padConst_rank(%arg0: tensor<13x21xf32>, %arg1: tenso
// -----
+func.func @test_pad_padding_shape_mismatch(%arg0: tensor<13x21x3xf32>, %arg1: tensor<2x2xi32>) -> tensor<13x21x3xf32> {
+ // expected-error@+1 {{'tosa.pad' op expect 'padding' tensor shape to match [rank(input), 2]}}
+ %0 = tosa.pad %arg0, %arg1 : (tensor<13x21x3xf32>, tensor<2x2xi32>) -> tensor<13x21x3xf32>
+ return %0 : tensor<13x21x3xf32>
+}
+
+// -----
+
+func.func @test_pad_padding_shape_mismatch(%arg0: tensor<13x21x3xf32>, %arg1: tensor<3x1xi32>) -> tensor<13x21x3xf32> {
+ // expected-error@+1 {{'tosa.pad' op expect 'padding' tensor shape to match [rank(input), 2]}}
+ %0 = tosa.pad %arg0, %arg1 : (tensor<13x21x3xf32>, tensor<3x1xi32>) -> tensor<13x21x3xf32>
+ return %0 : tensor<13x21x3xf32>
+}
+
+// -----
+
func.func @test_transpose_non_const(%arg0: tensor<13x21x3xf32>, %arg1: tensor<3xi32>) -> tensor<3x13x21xf32> {
// expected-error@+1 {{'tosa.transpose' op perms of transpose is not constant}}
%0 = tosa.transpose %arg0, %arg1 : (tensor<13x21x3xf32>, tensor<3xi32>) -> tensor<3x13x21xf32>
|
|
@llvm/pr-subscribers-mlir Author: Longsheng Mou (CoTinker) ChangesThis PR adds a missing verifier for Full diff: https://github.com/llvm/llvm-project/pull/120934.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index 631d3c48f2df02..6d9d2badc2970b 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -831,6 +831,11 @@ LogicalResult tosa::PadOp::verify() {
if (paddingType.hasRank() && paddingType.getRank() != 2)
return emitOpError() << "expect 'padding' tensor rank equal to 2.";
+ auto paddingShape = paddingType.getShape();
+ if (paddingShape.front() != inputType.getRank() || paddingShape.back() != 2)
+ return emitOpError()
+ << "expect 'padding' tensor shape to match [rank(input), 2]";
+
return success();
}
diff --git a/mlir/test/Dialect/Tosa/invalid.mlir b/mlir/test/Dialect/Tosa/invalid.mlir
index cca50b25d14d6b..8a8fcbed6a67f3 100644
--- a/mlir/test/Dialect/Tosa/invalid.mlir
+++ b/mlir/test/Dialect/Tosa/invalid.mlir
@@ -119,6 +119,22 @@ func.func @test_pad_invalid_padConst_rank(%arg0: tensor<13x21xf32>, %arg1: tenso
// -----
+func.func @test_pad_padding_shape_mismatch(%arg0: tensor<13x21x3xf32>, %arg1: tensor<2x2xi32>) -> tensor<13x21x3xf32> {
+ // expected-error@+1 {{'tosa.pad' op expect 'padding' tensor shape to match [rank(input), 2]}}
+ %0 = tosa.pad %arg0, %arg1 : (tensor<13x21x3xf32>, tensor<2x2xi32>) -> tensor<13x21x3xf32>
+ return %0 : tensor<13x21x3xf32>
+}
+
+// -----
+
+func.func @test_pad_padding_shape_mismatch(%arg0: tensor<13x21x3xf32>, %arg1: tensor<3x1xi32>) -> tensor<13x21x3xf32> {
+ // expected-error@+1 {{'tosa.pad' op expect 'padding' tensor shape to match [rank(input), 2]}}
+ %0 = tosa.pad %arg0, %arg1 : (tensor<13x21x3xf32>, tensor<3x1xi32>) -> tensor<13x21x3xf32>
+ return %0 : tensor<13x21x3xf32>
+}
+
+// -----
+
func.func @test_transpose_non_const(%arg0: tensor<13x21x3xf32>, %arg1: tensor<3xi32>) -> tensor<3x13x21xf32> {
// expected-error@+1 {{'tosa.transpose' op perms of transpose is not constant}}
%0 = tosa.transpose %arg0, %arg1 : (tensor<13x21x3xf32>, tensor<3xi32>) -> tensor<3x13x21xf32>
|
There was a problem hiding this comment.
Thought that padding is of rank 1 and dimensions size equal to 2 * rank(input).
There was a problem hiding this comment.
The Spec says the type is shape_t<[2*rank(shape1)]>, I'm not sure it's 1D or 2D. But I find that the padding of tosa.pad before is all 2D.
There was a problem hiding this comment.
The expectation changed between v0.80 and v1.0.0 of the specification:
- v0.80 -> rank
2, shape[rank(shape1),2] - v1.0.0 -> rank
1, shape[2*rank(shape1)]
Hopefully this helps with some confusion here
There was a problem hiding this comment.
Thanks for your reply , I understand it. I'll fix my PR follows the v1.0 Spec.
|
I'm sorry, I just got the wrong branch and uploaded unrelated code, I'll deal with it later. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
CoTinker
force-pushed
the
pad
branch
2 times, most recently
from
917b983 to
687df6c
Compare
This PR adds a missing verifier for `tosa.pad`, ensuring that the padding shape matches [2*rank(input)] according to V1.0 specification.
GeorgeARM
left a comment
GeorgeARM
left a comment
There was a problem hiding this comment.
Great work @CoTinker! Am happy with this change but would like for @sjarus and @eric-k256 to have a look as well as this will break existing legalizations to TOSA.
|
Okay, thanks. |
|
Hello @CoTinker , first of all THANK YOU - this is a pretty significant contribution and on a first review it looks fine. We only released the TOSA 1.0 draft recently (https://discourse.llvm.org/t/rfc-tosa-dialect-increment-to-v1-0/83708/1) so the work on updating the dialect to it is very welcome. As mentioned earlier, the only hurdle here is aligning the LLVM and TensorFlow repositories - the latter picks up the former and contains lowerings to TOSA that would break CI unless coordinated effectively. @leandron and @jpienaar are currently engaged in ensuring this happens in a smooth manner, so wold you kindly wait until this is addressed before landing this change ? |
Of course, I'll wait it until them address the inconsistent behaviour. Thanks for your review. And could you please let me know when all this work is done? |
sjarus
left a comment
sjarus
left a comment
There was a problem hiding this comment.
@CoTinker this is approved. Please proceed and land it - if you need help with doing so, I can do it on your behalf.
Also, I'd like to call out this contribution on the TOSA 1.0 RFC announcement if you don't mind: https://discourse.llvm.org/t/rfc-tosa-dialect-increment-to-v1-0/83708
5 participants
This PR adds a missing verifier for
tosa.pad, ensuring that the padding shape matches [2*rank(shape1)] according to V1.0.0 Specification. Fixes #119840.